Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Vacated Decision Issues from Request Issues #13173

Merged
merged 41 commits into from
Feb 7, 2020

Conversation

jcq
Copy link
Contributor

@jcq jcq commented Jan 16, 2020

Connects #13069

Description

This adds code to create vacated decision issues from given request issues

JC Quirin and others added 21 commits December 18, 2019 12:41
…active atty to pass

(ensure variables are created in necessary order)
…nUpdater rather than on frontend;

additional fields saved in newly created request issues;
@codeclimate
Copy link

codeclimate bot commented Jan 16, 2020

Code Climate has analyzed commit 0d24049 and detected 0 issues on this pull request.

View more on Code Climate.

JC Quirin added 2 commits January 15, 2020 17:30
…cisionMotion;

moved `create_vacated_decision_issue` in RequestIssue to make method accessible;
Copy link
Contributor

@leikkisa leikkisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some thoughts and proposals below. Let me know if you have any questions.

app/models/post_decision_motion.rb Outdated Show resolved Hide resolved
app/models/post_decision_motion.rb Outdated Show resolved Hide resolved
app/models/request_issue.rb Show resolved Hide resolved
spec/models/post_decision_motion_spec.rb Outdated Show resolved Hide resolved
db/schema.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@leikkisa leikkisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm prematurely adding a few comments for when this is adapted to the new appeal stream flow, after #13325

@@ -160,7 +160,7 @@ def associated_request_issue
end

def create_contesting_request_issue!
RequestIssue.create!(
RequestIssue.find_or_create_by!(
decision_review: decision_review,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are now going to be added onto the new appeal stream instead of the original appeal

app/models/request_issue.rb Show resolved Hide resolved
@@ -25,6 +25,17 @@ def return_to_lit_support
render json: { tasks: ::WorkQueue::TaskSerializer.new(appeal_tasks, is_collection: true) }
end

def create_issues
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to move creating issues to the PostDecisionMotionUpdater, when the new appeal stream is getting created

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if their creation is automatic we don't need an endpoint for it.

Methinks it makes sense to wait until your PR (#13325) lands and then incorporate upon new stream creation.

Copy link
Contributor

@leikkisa leikkisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey JC - it looks like there's some "clean up" from the previous iteration of the code left, but besides that, this PR is looking good, so I'll go ahead and give it the approval now.

@@ -160,8 +160,9 @@ def associated_request_issue
end

def create_contesting_request_issue!
RequestIssue.create!(
decision_review: decision_review,
vacate_appeal_stream = Appeal.find_by(stream_type: "vacate", stream_docket_number: decision_review.docket_number)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good for now, we'll probably update it in Sandra's ticket to pass in the decision review so it can be used for De Novo's too.

config/routes.rb Outdated Show resolved Hide resolved
spec/controllers/post_decision_motions_controller_spec.rb Outdated Show resolved Hide resolved
@@ -65,6 +65,8 @@
expect(task.reload.status).to eq Constants.TASK_STATUSES.completed
verify_vacate_stream
end

it "should create decision issues on new vacate"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a stray line

expect(request_issues.size).to eq(motion.decision_issues_for_vacatur.size)

decision_issues = vacate_stream.decision_issues
expect(decision_issues.size).to eq(motion.decision_issues_for_vacatur.size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jcq jcq added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Feb 7, 2020
@va-bot va-bot merged commit 01c19f7 into master Feb 7, 2020
@va-bot va-bot deleted the jc/13069-mtv-create-decision-issues branch February 7, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master Team: Foxtrot 🦊
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants